plugins: ensure to log the offending plugin on instantiation failure #5739
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
wisp3rwind
force-pushed
the
plugin-traceback
branch
from
3a4a4b7 to
db32fa1
Compare
wisp3rwind
force-pushed
the
plugin-traceback
branch
5 times, most recently
from
bde4397 to
2129eb4
Compare
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
|
This should be ready to go now; it's easiest to review commit-by-commit. |
wisp3rwind
force-pushed
the
plugin-traceback
branch
from
2129eb4 to
cc877e5
Compare
wisp3rwind
force-pushed
the
plugin-traceback
branch
from
cc877e5 to
8a862ba
Compare
minimize how much code is wrapped in a given try...except statement
…roken error handling from a quick test, it appears that the old code shouldn't even be working anymore due to additional quotes in exc.args[0]
also remove a unnecessary check before adding to a set
without an argument (i.e. the default `names=()`), this was a no-op actual plugin loading is triggered by tests, or by the ui code -> Turns out it's not an actual no-op in our testing code due to patching with attaching some side-effects to load_plugins... This is fixed up in the next commit.
Instead of the first call to find_plugins(). This could change behavior slightly, since plugin load (and errors that happen in the process) could occur earlier now. But: - I'd argue this is better anyway, since plugin load errors now happen in the initial call to load_plugins() instead of somewhere in the middle of the first operation that uses plugins. - In our ui code, both happen immediately after each other anyway: ui._setup calls 1) ui._load_plugins -> plugins.load_plugins 2) plugins.types, plugins.named_queries, ...
- better error handling (includes restoring some unintended changes) - cleanup of some traces of the previous architecture of the plugin loader
wisp3rwind
force-pushed
the
plugin-traceback
branch
from
8a862ba to
1935488
Compare
|
Should be good to go, but I'll rebase on #5841 once its merged. Note that this isn't quite what the title says anymore: Logging wasn't as incomplete as I thought in the context of #5701. Thus, this PR only slightly improves logging, notably by fixing the "not found" error case (detection of which was quite hacky before, and in fact broken), and by logging the fully-qualified path to offending plugin classes. Additionally, the PR refactors plugin loading, reducing nesting of the code, and actually instantiating plugins in |
| if cls in _instances: | ||
| log.debug( | ||
| "repeated initialization of plugin class {}.{}", | ||
| cls.__module__, | ||
| cls.__qualname__, | ||
| ) |
There was a problem hiding this comment.
| if cls in _instances: | |
| log.debug( | |
| "repeated initialization of plugin class {}.{}", | |
| cls.__module__, | |
| cls.__qualname__, | |
| ) |
Let's remove this check and ensure uniqueness with
for name in set(names):
...on line 357.
Came up in #5701 (comment) where an error can't easily be traced back to a plugin.
(Draft because I want to avoid creating merge conflicts for #5701 before that is merged
, and because I might a few additional improvements here.)